Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

timers: add hasRef method to Timeout & Immediate #20898

Closed
wants to merge 2 commits into from
Closed

timers: add hasRef method to Timeout & Immediate #20898

wants to merge 2 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented May 22, 2018 •
edited
Loading

Provide a way to check whether the current timer or immediate is refed. This was done as per request from @Fishrock123 due to the removal of _handle from unrefed timers.

The method name was chosen to match what used to be available on the _handle — unlike a getter it should also make it clear that it's not possible to change it by trying to assign.

(Since it depends on a semver-major PR, I'm labeling this as such too.)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@apapirovski
timers: add hasRef method to Timeout & Immediate
ebb23e4 
Provide a way to check whether the current timer or immediate
is refed.
@apapirovski apapirovski added timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. semver-major PRs that contain breaking changes and should be released in the next major version. labels May 22, 2018
@nodejs-github-bot nodejs-github-bot added the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label May 22, 2018
@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/15040/

@addaleax addaleax added semver-minor PRs that contain new features and should be released in the next minor version. dont-land-on-v8.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels May 22, 2018
jasnell
jasnell reviewed May 23, 2018
View reviewed changes
doc/api/timers.md
@@ -23,6 +23,16 @@ running as long as the immediate is active. The `Immediate` object returned by
[`setImmediate()`][] exports both `immediate.ref()` and `immediate.unref()`
functions that can be used to control this default behavior.

### immediate.hasRef()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... maybe isRef()? hasRef just seems... awkward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isRefed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasRef matches the HandleWrap, hence that choice. Open to other names if there's a consensus.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really mind what is chosen

TimothyGu
TimothyGu approved these changes May 23, 2018
View reviewed changes
doc/api/timers.md
@@ -23,6 +23,16 @@ running as long as the immediate is active. The `Immediate` object returned by
[`setImmediate()`][] exports both `immediate.ref()` and `immediate.unref()`
functions that can be used to control this default behavior.

### immediate.hasRef()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isRefed?

cjihrig
cjihrig approved these changes May 23, 2018
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a few nits.

doc/api/timers.md Outdated

* Returns: {boolean}

Used to check whether the `Immediate` object will keep the Node.js event loop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe replace "Used to check whether ..." with "If true, ..."

doc/api/timers.md Outdated

* Returns: {boolean}

Used to check whether the `Timeout` object will keep the Node.js event loop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here.

test/parallel/test-timers-immediate-unref.js Outdated
const assert = require('assert');

const immediate = setImmediate(() => {});
assert(immediate.hasRef());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use assert.strictEqual() in these tests.

@apapirovski
Copy link
Member Author

Updated. CI: https://ci.nodejs.org/job/node-test-pull-request/15054/

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 25, 2018
Fishrock123
Fishrock123 approved these changes Jun 1, 2018
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really mind what name it gets, personally.

@apapirovski
Copy link
Member Author

Landed in 48a2568

@apapirovski apapirovski closed this Jun 1, 2018
@apapirovski apapirovski deleted the patch-timers-has-ref branch June 1, 2018 08:49
apapirovski added a commit that referenced this pull request Jun 1, 2018
@apapirovski
timers: add hasRef method to Timeout & Immediate
48a2568 
Provide a way to check whether the current timer or immediate is refed.

PR-URL: #20898
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@TimothyGu TimothyGu TimothyGu approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
semver-minor PRs that contain new features and should be released in the next minor version. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@apapirovski @jasnell @addaleax @Fishrock123 @TimothyGu @cjihrig @BridgeAR @targos @nodejs-github-bot